Implemented easydict. #1094

odoku · 2020-06-16T04:02:18Z

What this patch does to fix the issue.

CLAassistant · 2020-06-16T04:02:23Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ odoku
❌ bo-mergebot[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bo-code-review-bot · 2020-06-16T04:02:24Z

This PR needs Approvals as follows.

Ownership Approval for / from iizukak, tkng, ruimashita
Readability Approval for Python from tkng, tsawada, tfujiwar

Please choose reviewers and requet reviews!

Click to see how to approve each reviews

You can approve this PR by triggered comments as follows.

Approve all reviews requested to you (readability and ownership) and LGTM review
Approval, LGTM
Approve all ownership reviews
Ownership Approval or OA
Approve all readability reviews
Readability Approval or RA
Approve specified review targets
- Example of Ownership Reviewer of /: Ownership Approval for / or OA for /
- Example of Readability Reviewer of Python: Readability Approval for Python or RA for Python
Approve LGTM review
LGTM

See all trigger comments

Please replace [Target] to review target

Ownership Approval
- Ownership Approval for [Target]
- OA for [Target]
- Ownership Approval
- OA
- Approval
Readability Approval
- Readability Approval for [Target]
- RA for [Target]
- [Target] Readability Approval
- [Target] RA
- Readability Approval
- RA
- Approval
LGTM
- LGTM
- lgtm

iizukak

Looks good except license header.

iizukak · 2020-06-17T01:31:40Z

blueoil/utils/smartdict.py

@@ -0,0 +1,30 @@
+class SmartDict(dict):


Please add license header.

iizukak · 2020-06-17T01:31:48Z

tests/unit/util_tests/test_smartdict.py

@@ -0,0 +1,63 @@
+import pytest


iizukak · 2020-06-17T01:35:09Z

@tfujiwar Please review 🙇

a-hanamoto · 2020-06-17T09:04:34Z

I'll check tomorrow so please wait for a while. Thanks!

a-hanamoto · 2020-06-18T09:28:04Z

blueoil/utils/smartdict.py

+        super(SmartDict, self).__init__()
+        self.update(d or {}, **kwargs)
+
+    def update(self, d=None, **kwargs):


Can we make this method non-public? _update

@a-hanamoto
Since the update function is in dict, I think it is necessary for SmartDict which inherits dict.
https://docs.python.org/ja/3/library/stdtypes.html?highlight=dict#dict.update

Then could you please write test cases for update?

I added test case for update function. Please review it.

a-hanamoto · 2020-06-18T09:39:00Z

blueoil/utils/smartdict.py

+class SmartDict(dict):
+    def __init__(self, d=None, **kwargs):
+        super(SmartDict, self).__init__()
+        self.update(d or {}, **kwargs)


d will be replaced d or {} soon after calling update, so d is OK, I think.

a-hanamoto · 2020-06-18T09:57:47Z

blueoil/utils/smartdict.py

+    def __setitem__(self, name, value):
+        if isinstance(value, (list, tuple)):
+            value = [
+                self.__class__(x) if isinstance(x, dict) else x


SmartDict? I wonder why there are both of self.__class__ and SmartDict

@a-hanamoto
Your question is, why do you write "self.class(x)" instead of "SmartDict(x)"?
This is because if you inherit "SmartDict", "self.class" will be a subclass.

class HyperSmartDict(SmartDict): pass d = HyperSmartDict() d.__class__ == HyperSmartDict

Got your point. Thanks!

a-hanamoto · 2020-06-18T10:35:16Z

blueoil/utils/smartdict.py

+        super(SmartDict, self).__init__()
+        self.update(d or {}, **kwargs)
+
+    def update(self, d=None, **kwargs):


Then could you please write test cases for update?

a-hanamoto · 2020-06-19T03:43:34Z

blueoil/utils/smartdict.py

+    def __setitem__(self, name, value):
+        if isinstance(value, (list, tuple)):
+            value = [
+                self.__class__(x) if isinstance(x, dict) else x


Got your point. Thanks!

a-hanamoto

RA (reverse-shadowing)
Thank you!

tfujiwar · 2020-06-21T08:26:42Z

blueoil/utils/smartdict.py

+    def __getattr__(self, name):
+        if name in self:
+            return self[name]
+        return super(SmartDict, self).__getattr__(name)


I think __getattr__ should raise an AttributeError instead of delegating to the super class. Because Dict doesn't have a method __getattr__, the current error message is like:

AttributeError: 'super' object has no attribute '__getattr__'

It should be like:

AttributeError: 'SmartDict' object has no attribute 'x'

tfujiwar · 2020-06-21T08:34:31Z

blueoil/utils/smartdict.py

+        if hasattr(self, name):
+            return super(SmartDict, self).__setattr__(name, value)
+        self[name] = value


I think self[name] = value is enough. Currently, the results are different for an existing and non-existing key.

d = SmartDict(a=None) d.a = [{"k":"v"}] d.b = [{"k":"v"}] d.a[0].__class__ # dict d.b[0].__class__ # SmartDict

tfujiwar

RA
Thanks for revising.

iizukak · 2020-06-25T04:48:43Z

/ready

bo-mergebot · 2020-06-25T04:48:44Z

⏳Merge job is queued...

bo-mergebot · 2020-06-25T04:48:45Z

😥This PR is not approved yet.

iizukak · 2020-06-25T04:53:03Z

OA

iizukak · 2020-06-25T04:53:08Z

/ready

bo-mergebot · 2020-06-25T04:53:09Z

⏳Merge job is queued...

Implemented easydict.

1f2d32b

blueoil-butler bot added the CI: auto-run Run CI automatically label Jun 16, 2020

odoku self-assigned this Jun 16, 2020

odoku added 2 commits June 16, 2020 13:21

Fixed linter error.

089404b

Renamed to SmartDict.

933b782

iizukak approved these changes Jun 17, 2020

View reviewed changes

iizukak requested a review from tfujiwar June 17, 2020 01:34

tfujiwar requested review from a-hanamoto and removed request for tfujiwar June 17, 2020 02:24

Added license text.

1ab2e47

Fixed lint error.

8be3a58

a-hanamoto suggested changes Jun 18, 2020

View reviewed changes

Removed unnecessary syntax.

811782b

odoku requested a review from a-hanamoto June 18, 2020 10:25

a-hanamoto suggested changes Jun 19, 2020

View reviewed changes

Added test for update function.

a5a61bd

odoku requested a review from a-hanamoto June 19, 2020 07:41

a-hanamoto approved these changes Jun 19, 2020

View reviewed changes

a-hanamoto requested a review from tfujiwar June 19, 2020 13:10

tfujiwar suggested changes Jun 21, 2020

View reviewed changes

Fixed get/set attribute methods.

370fe86

odoku requested a review from tfujiwar June 24, 2020 11:39

tfujiwar approved these changes Jun 24, 2020

View reviewed changes

Merge branch 'master' into reimplement-easydict

fc5979b

bo-mergebot bot merged commit aee0301 into blue-oil:master Jun 25, 2020

odoku deleted the reimplement-easydict branch June 25, 2020 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented easydict. #1094

Implemented easydict. #1094

odoku commented Jun 16, 2020 •

edited

Loading

CLAassistant commented Jun 16, 2020 •

edited

Loading

bo-code-review-bot bot commented Jun 16, 2020

iizukak left a comment

iizukak Jun 17, 2020

iizukak Jun 17, 2020

iizukak commented Jun 17, 2020

a-hanamoto commented Jun 17, 2020

a-hanamoto Jun 18, 2020

odoku Jun 18, 2020

a-hanamoto Jun 18, 2020

odoku Jun 19, 2020

a-hanamoto Jun 18, 2020

a-hanamoto Jun 18, 2020

odoku Jun 18, 2020

a-hanamoto Jun 19, 2020

a-hanamoto Jun 18, 2020

a-hanamoto Jun 19, 2020

a-hanamoto left a comment

tfujiwar Jun 21, 2020

tfujiwar Jun 21, 2020

tfujiwar left a comment

iizukak commented Jun 25, 2020

bo-mergebot bot commented Jun 25, 2020

bo-mergebot bot commented Jun 25, 2020

iizukak commented Jun 25, 2020

iizukak commented Jun 25, 2020

bo-mergebot bot commented Jun 25, 2020

Implemented easydict. #1094

Implemented easydict. #1094

Conversation

odoku commented Jun 16, 2020 • edited Loading

What this patch does to fix the issue.

CLAassistant commented Jun 16, 2020 • edited Loading

bo-code-review-bot bot commented Jun 16, 2020

iizukak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iizukak commented Jun 17, 2020

a-hanamoto commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hanamoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfujiwar left a comment

Choose a reason for hiding this comment

iizukak commented Jun 25, 2020

bo-mergebot bot commented Jun 25, 2020

bo-mergebot bot commented Jun 25, 2020

iizukak commented Jun 25, 2020

iizukak commented Jun 25, 2020

bo-mergebot bot commented Jun 25, 2020

odoku commented Jun 16, 2020 •

edited

Loading

CLAassistant commented Jun 16, 2020 •

edited

Loading